Skip to content

Fix: Add tenant ID validation in UserPrincipalManager#49631

Merged
rujche merged 15 commits into
mainfrom
rujche/main/fix-icm-issue-about-UserPrincipalManager
Jun 29, 2026
Merged

Fix: Add tenant ID validation in UserPrincipalManager#49631
rujche merged 15 commits into
mainfrom
rujche/main/fix-icm-issue-about-UserPrincipalManager

Conversation

@rujche

@rujche rujche commented Jun 25, 2026

Copy link
Copy Markdown
Member

Description

This PR fixes a cross-tenant authentication bypass vulnerability in Azure SDK for Java by implementing proper tenant ID (tid claim) validation in JWT tokens.

Problem

The AAD authentication filters (AadAuthenticationFilter and AadAppRoleStatelessAuthenticationFilter) were not validating the tenant ID claim in JWT tokens, allowing tokens from different tenants to be accepted, creating a security vulnerability.

Solution

  • Implemented tenant ID claim validation against the configured tenant
  • Tokens are now rejected if the tid claim doesn't match the configured tenant ID
  • Maintains backward compatibility by skipping validation when tenant is not configured
  • Supports multi-tenant configurations (common, organizations, consumers) with validation bypass
  • Added comprehensive logging to inform users about verification status

Changes

New File: AadJwtClaimsVerifier.java

  • Custom JWT claims verifier extending DefaultJWTClaimsVerifier
  • Encapsulates issuer, audience, and tenant ID validation logic
  • Validates tenant ID claim with security hardening:
    • Locale.ROOT for case-insensitive normalization
    • Safe type casting to prevent ClassCastException on malicious tokens
  • Skips validation for multi-tenant configurations (common, organizations, consumers)
  • Debug logging for verification status

Updated: UserPrincipalManager.java

  • Simplified to use new AadJwtClaimsVerifier class
  • Added package-private constructor for testing
  • Maintains all existing functionality
  • Cleaner separation of concerns

Updated: UserPrincipalManagerTests.java

  • Added 4 new unit tests for tenant ID validation:
    • tenantIdValidationSucceedsWhenMatchingConfiguredTenant()
    • tenantIdValidationFailsWhenMismatchedTenant()
    • tenantIdValidationSkippedWhenNoTenantConfigured()
    • tenantIdValidationSkippedWhenOrganizationsConfigured()
  • Uses package-private constructor to avoid reflective mutation
  • All 9 tests pass successfully (5 original + 4 new)

Updated: CHANGELOG.md

  • Documented the security fix for 7.4.0-beta.1 release

Security Impact

  • Severity: High - Prevents cross-tenant authentication bypass
  • Scope: Affects AadAuthenticationFilter and AadAppRoleStatelessAuthenticationFilter
  • Backward Compatibility: Maintained - validation only enforced when tenant ID is configured
  • Multi-tenant Support: Preserved - multi-tenant endpoints (common, organizations, consumers) skip validation

Testing

  • All existing tests pass
  • 4 new comprehensive unit tests added
  • No network dependencies required
  • Tests cover happy path, error cases, and multi-tenant scenarios

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

…bypass

- Validate the 'tid' (tenant ID) claim in JWT tokens against the configured tenant
- Only accept tokens from the configured tenant if tenant ID is specified
- Maintains backward compatibility by skipping validation when tenant is not configured
- Fixes the authentication bypass vulnerability in UserPrincipalManager

Changes:
1. UserPrincipalManager.java:
   - Added tenant ID claim validation in getValidator() method
   - Extract configured tenant ID from AadAuthenticationProperties
   - Reject tokens where tid doesn't match configured tenant
   - Added debug logging for successful tenant validation
   - Added org.springframework.util.StringUtils import

2. UserPrincipalManagerTests.java:
   - Added 4 new unit tests for tenant ID validation:
     * tenantIdValidationSucceedsWhenMatchingConfiguredTenant()
     * tenantIdValidationFailsWhenMismatchedTenant()
     * tenantIdValidationSkippedWhenNoTenantConfigured()
     * tenantIdValidationSkippedWhenTenantPropertyIsEmpty()
   - Added reflection helper methods for testing private methods
   - All 9 tests pass successfully (5 original + 4 new)
Copilot AI review requested due to automatic review settings June 25, 2026 01:27
@rujche rujche requested review from a team, Netyyyy, moarychan and saragluna as code owners June 25, 2026 01:27
@github-actions github-actions Bot added the azure-spring All azure-spring related issues label Jun 25, 2026
@rujche rujche self-assigned this Jun 25, 2026
@rujche rujche added the azure-spring-aad Spring active directory related issues. label Jun 25, 2026
@rujche rujche moved this from Todo to In Progress in Spring Cloud Azure Jun 25, 2026
@rujche rujche added this to the 2026-07 milestone Jun 25, 2026
Add entry for the cross-tenant authentication bypass fix in AAD authentication filters to the 7.4.0-beta.1 release notes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds JWT tid (tenant ID) claim validation to UserPrincipalManager to prevent cross-tenant token acceptance when a single tenant is intended, along with unit tests covering matching/mismatching/skip scenarios.

Changes:

  • Added tid vs configured tenant validation inside UserPrincipalManager#getValidator()’s claims verifier.
  • Added new unit tests covering tenant match/mismatch and validation skip behavior.
  • Introduced reflection-based helpers in tests to reach private implementation details.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
sdk/spring/spring-cloud-azure-autoconfigure/src/main/java/com/azure/spring/cloud/autoconfigure/implementation/aad/filter/UserPrincipalManager.java Adds tid claim validation against configured tenant.
sdk/spring/spring-cloud-azure-autoconfigure/src/test/java/com/azure/spring/cloud/autoconfigure/implementation/aad/filter/UserPrincipalManagerTests.java Adds tests + reflection helpers to validate the new tenant-checking behavior.

- Skip tenant ID validation for multi-tenant values (common, organizations, consumers)
- Normalize tenant IDs to lowercase for case-insensitive comparison
- Fix test helper method to properly accept and use JWSAlgorithm parameter
- Update tests to reflect actual Spring behavior (default 'common' for unconfigured tenant)
- Use 'organizations' instead of empty string for multi-tenant test scenario

These changes ensure:
1. Multi-tenant applications (using common/organizations/consumers endpoints) still work
2. Tenant ID comparison is case-insensitive and normalized
3. Tests accurately reflect real-world Spring configuration defaults
4. Code is cleaner and follows best practices

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

- Use Locale.ROOT for security-sensitive tenant ID normalization (prevents locale-dependent behavior)
- Change direct String cast to Object cast with toString() to safely handle non-String tid claims
- Rename test method from tenantIdValidationSkippedWhenTenantPropertyIsEmpty() to tenantIdValidationSkippedWhenOrganizationsConfigured() for clarity
- Maintains consistency with AadResourceServerConfiguration#getTrimmedTenantId patterns
- Improves robustness against malicious tokens with unexpected claim types

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

…h rule

- Reformatted normalizedTokenTid initialization to split the ternary operator across multiple lines
- Ensures line 236 stays under 120 characters as enforced by Checkstyle
- All 9 tests pass, BUILD SUCCESS

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

…n of final fields

- Added UserPrincipalManager(JWKSource, AadAuthenticationProperties) package-private constructor
- Updated tenant ID validation tests to use the new constructor instead of reflection
- Removed setAadAuthenticationProperties() helper method that modified private final fields
- Tests now avoid brittle reflection and are cleaner
- All 9 tests pass, BUILD SUCCESS
@rujche rujche requested a review from Copilot June 25, 2026 02:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

- Make AadJwtClaimsVerifier package-private and final (internal implementation detail)
- Extract issuer validation logic into AadIssuerValidator utility class to prevent duplication
- Update UserPrincipalManager to use AadIssuerValidator instead of local isAadIssuer method
- Add unit test tenantIdValidationSkippedWhenConsumersConfigured to cover 'consumers' multi-tenant value
- Remove duplicate issuer prefix constants from UserPrincipalManager

These changes improve code reusability, reduce duplication in security validation logic, and ensure consistent issuer validation across the package.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

rujche added 2 commits June 26, 2026 14:00
- Handle null/empty audience list gracefully with BadJWTException instead of NPE
- Remove trailing whitespace from blank lines in test file

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

… reflection in tests

Tests now directly instantiate AadJwtClaimsVerifier (same package), eliminating
the need for the package-private UserPrincipalManager constructor and the
reflection-based getValidator() helper.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@rujche

rujche commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

/azp run java - spring - tests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@rujche rujche merged commit 3b4bc34 into main Jun 29, 2026
131 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Spring Cloud Azure Jun 29, 2026
@rujche rujche deleted the rujche/main/fix-icm-issue-about-UserPrincipalManager branch June 29, 2026 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

azure-spring All azure-spring related issues azure-spring-aad Spring active directory related issues.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants